-
Notifications
You must be signed in to change notification settings - Fork 747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clone state ahead of block production #4925
Clone state ahead of block production #4925
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great optimisation! I just have a question about pruning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I just have a suggestion about avoiding holding two states in memory. I'm happy with the PR as-is though.
// If we're proposing, clone the head state preemptively so that it isn't on | ||
// the hot path of proposing. We can delete this once we have tree-states. | ||
if let Some(proposer_head) = proposer_head { | ||
if let Some(proposer_state) = beacon_chain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be beneficial to drop the block_production_state
above this line. It would prevent us from holding the old state in memory whilst we load the new one.
The downside would be that there are some times where we clear the block_production_state
when we don't need to (i.e. the state is not in the snapshot cache but would be useful for the next proposal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I've added that in 6d5d558. It will still hold 2 states in memory if the beacon node has two proposals two slots in a row on the same head block, but I think this is reasonably obscure enough to be tolerated until we get tree-states. I'll try to deploy that change on Holesky in a few hours so I can confirm it didn't regress on the perf front.
I think this is good to merge. No noticeable memory spikes on the Holesky node where it's running, and block production times continue to show a ~500ms reduction. OK to merge @paulhauner ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Merge at will!
Proposed Changes
When proposing, clone the state for the proposal 500ms before the start of the slot and cache it. We then move this clone into position when the request to build a block comes in.
This should shave 250-500ms off block production on larger networks like mainnet and Holesky. We won't need this when we have tree-states, but this is a relatively simple patch to keep us competitive until then.